Skip to content

Conversation

benjaminjohnson01
Copy link
Contributor

@benjaminjohnson01 benjaminjohnson01 commented Sep 6, 2025

Fixes gh-138514.


📚 Documentation preview 📚: https://cpython-previews--138591.org.readthedocs.build/

@python-cla-bot
Copy link

python-cla-bot bot commented Sep 6, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@picnixz picnixz changed the title getpass: restrict echo_char to a single ASCII character (gh-138514) gh-138514: getpass: restrict echo_char to a single ASCII character Sep 6, 2025
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some tests?

@benjaminjohnson01
Copy link
Contributor Author

Yes! Thank you for the feedback and review. I intended to work on this some more. I will add tests and look for potential efficiencies made possible by no longer accepting multi-character strings.

Co-authored-by: Bénédikt Tran <[email protected]>
@picnixz
Copy link
Member

picnixz commented Sep 9, 2025

Note that we have 3.14-rc3 coming next week so I would like for this to be merged before if possible. If you don't have enough time, I can take over.

@benjaminjohnson01
Copy link
Contributor Author

benjaminjohnson01 commented Sep 10, 2025

I can do this now! I apologize for the delay.

@benjaminjohnson01
Copy link
Contributor Author

benjaminjohnson01 commented Sep 10, 2025

I notice on Windows when an arrow key is used during password entering, two echo_char characters appear, and the password string saves the arrow key presses as control-ish characters (like "àK") due to how msvcrt.getwch() handles arrow keys. Should I address this behavior in this pull request (by ignoring/not accepting characters created through arrow key presses)? Or should I make a separate issue for this?

@benjaminjohnson01 benjaminjohnson01 marked this pull request as ready for review September 10, 2025 02:35
@picnixz
Copy link
Member

picnixz commented Sep 10, 2025

Does it also happen when there is no echo char?

@benjaminjohnson01 benjaminjohnson01 marked this pull request as draft September 10, 2025 15:21
@picnixz
Copy link
Member

picnixz commented Sep 11, 2025

However, is this an issue I should open? Using a password involving arrow keys provides a different password on Windows versus Unix systems. For example, with three left arrow key presses as a password...

I don't think it's an "issue". When I use sudo to enter my password and press left/right arrows, they are not interpreted as left/right arrows. It could be considered a feature if you think it's worth it but OTOH, I think it'll be too complex and really too niche. If the password is wrong, or if made a typo, you usually erase using backspace or re-enter the password. Should this feature be supported, a preliminary thread in https://discuss.python.org/c/ideas/6 shouold be opened.

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove the s to rejects and just use reject. It saves 1 char and we usually have test_[verb]* instead of test_[conjugated form]*.

@benjaminjohnson01
Copy link
Contributor Author

benjaminjohnson01 commented Sep 11, 2025

I don't think it's an "issue". When I use sudo to enter my password and press left/right arrows, they are not interpreted as left/right arrows. It could be considered a feature if you think it's worth it but OTOH, I think it'll be too complex and really too niche. If the password is wrong, or if made a typo, you usually erase using backspace or re-enter the password. Should this feature be supported, a preliminary thread in https://discuss.python.org/c/ideas/6 should be opened.

However, left/right arrows currently are being interpreted as left/right arrows in a way, right (since arrow keys are recorded as bytes/characters)? Instead of supporting this behavior, could we more simply remove it?

@benjaminjohnson01
Copy link
Contributor Author

benjaminjohnson01 commented Sep 11, 2025

I removed the s characters. Thank you once again for your feedback!

@picnixz
Copy link
Member

picnixz commented Sep 11, 2025

However, left/right arrows currently are being interpreted as left/right arrows in a way, right (since arrow keys are recorded as bytes/characters)?

I think they are interpreted that way because on Unix we use termios while on Windows it's provided by msvcrt.getwch() so we depend on that.

@picnixz
Copy link
Member

picnixz commented Sep 11, 2025

Unless there is a conflict to solve or a CI test to fix (that your changes wouldn't affect), avoid merging main into your branch as it wastes CI resources. TiA!

@benjaminjohnson01
Copy link
Contributor Author

benjaminjohnson01 commented Sep 11, 2025

Because I raise TypeError when not isinstance(echo_char, str), does the news blurb need to be updated?

@picnixz
Copy link
Member

picnixz commented Sep 11, 2025

Mmh, no. If we say "we expect a string", and you give something that is not a string, usually it raises a TypeError or an AttributeError. Currently passing something like 2 raises AttributeError because of echo_char.isprintable().

@benjaminjohnson01
Copy link
Contributor Author

@picnixz would you like me to make any additional modifications?

@picnixz
Copy link
Member

picnixz commented Sep 15, 2025

I'll take a look tomorrow during the sprint!

@picnixz picnixz merged commit ab6893a into python:main Sep 16, 2025
49 checks passed
@picnixz picnixz added the needs backport to 3.14 bugs and security fixes label Sep 16, 2025
@miss-islington-app
Copy link

Thanks @benjaminjohnson01 for the PR, and @picnixz for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 16, 2025
…acter (pythonGH-138591)

This amends commit bf8bbe9 by
restricting `echo_char` in `getpass.getpass` to single printable
ASCII characters as it would be uncommon to use long strings or
multi-byte characters for keyboard feedback.

---------
(cherry picked from commit ab6893a)

Co-authored-by: Benjamin Johnson <[email protected]>
Co-authored-by: Bénédikt Tran <[email protected]>
Co-authored-by: Brian Schubert <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Sep 16, 2025

GH-138988 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Sep 16, 2025
hugovk pushed a commit that referenced this pull request Sep 17, 2025
…racter (GH-138591) (#138988)

Co-authored-by: Benjamin Johnson <[email protected]>
Co-authored-by: Bénédikt Tran <[email protected]>
Co-authored-by: Brian Schubert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getpass's echo_char should be restricted to a single ASCII character
3 participants